Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Should not try to attach to modules that export primitives #83

Merged
merged 1 commit into from Nov 30, 2015
Merged

Should not try to attach to modules that export primitives #83

merged 1 commit into from Nov 30, 2015

Conversation

TheSavior
Copy link
Contributor

When people use rewire, they wouldn't typically try to explicitly rewire modules that export primitives.

However, since reweireify, rewire-global, and babel-plugin-rewire all get injected to every module, this needs to not blow up on objects that can't be attached to.

rewireify and rewire-global use Object.isExtensible instead of the type check. The behavior of Object.isExtensible change from es5 to es6. In ES5, it would throw if given something that was not an object. in ES6, it returns false. rewireify and rewire-global all run in the browser and all modern browsers have the es6 version of the functionality. Node 0.12 has the es5 functionality, so we need to use the type check instead.

I added a test to make sure that it doesn't blow up on modules that return sealed objects. Object.defineProperty will run and not throw, but won't actually modify the object.

@TheSavior
Copy link
Contributor Author

This PR makes all of my tests pass for rewire-global while reusing getImportGlobalsSrc and getDefinePropertySrc.

@TheSavior
Copy link
Contributor Author

@jhnns I rebased this from master. What are your thoughts on this? I'd like to be able to make these modules share more and have a consistent API.

@TheSavior
Copy link
Contributor Author

@jhnns Bump.

@jhnns
Copy link
Owner

jhnns commented Nov 30, 2015

In ES5, it would throw if given something that was not an object

Does this mean that null will throw on node 0.12?

@jhnns
Copy link
Owner

jhnns commented Nov 30, 2015

Btw: Thx for your PR 👍. I'll merge it asap.

@TheSavior
Copy link
Contributor Author 

$ nvm use 0.12
Now using node v0.12.7
$ node
> Object.isExtensible(null)
TypeError: Object.isExtensible called on non-object
    at Function.isExtensible (native)
    at repl:1:8
    at REPLServer.defaultEval (repl.js:132:27)
    at bound (domain.js:254:14)
    at REPLServer.runBound [as eval] (domain.js:267:12)
    at REPLServer.<anonymous> (repl.js:279:12)
    at REPLServer.emit (events.js:107:17)
    at REPLServer.Interface._onLine (readline.js:214:10)
    at REPLServer.Interface._line (readline.js:553:8)
    at REPLServer.Interface._ttyWrite (readline.js:830:14)
> 
(^C again to quit)
> 
$ nvm use node
Now using node v5.0.0
$ node
> Object.isExtensible(null)
false

@TheSavior
Copy link
Contributor Author

Good catch. Added a test case for null.

jhnns added a commit that referenced this pull request Nov 30, 2015
@jhnns
Merge pull request #83 from TheSavior/primitives
e124971 
Should not try to attach to modules that export primitives
@jhnns jhnns merged commit e124971 into jhnns:master Nov 30, 2015
@jhnns
Copy link
Owner

jhnns commented Nov 30, 2015

Shipped with 2.5.1

@TheSavior TheSavior deleted the primitives branch November 30, 2015 23:02
@TheSavior TheSavior mentioned this pull request Feb 9, 2016
Closed
@snyk-bot snyk-bot mentioned this pull request Mar 18, 2020
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@TheSavior @jhnns